Repository navigation
fix(tui): support SOCKS5 proxies and stop ALL_PROXY from crashing startup - #465
Merged
Merged
Conversation
…rtup Fixes #393. ALL_PROXY=socks5://... used to abort CLI startup because only http:// and https:// proxy URLs were accepted. - Route socks5://, socks5h://, and socks:// proxies through undici's Socks5ProxyAgent, which sends host names to the proxy for resolution. socks5h:// is rewritten to socks5:// with its credentials preserved. NO_PROXY hosts, including loopback, still connect directly. - Ignore an unusable ALL_PROXY fallback (for example socks4:// or a malformed URL) with a single stderr warning instead of failing startup. Explicit invalid HTTP_PROXY/HTTPS_PROXY values are still rejected. - Mask proxy credentials in warnings. - Suppress only undici's one-time SOCKS5 ExperimentalWarning while the agent is constructed; other warnings are unaffected. - Add unit tests and a loopback SOCKS5 integration test covering no-auth, username/password auth, domain-name CONNECT, and NO_PROXY.
Follow-up to #393 review feedback. - Dial SOCKS5 proxies at IPv6 literals such as socks5://[::1]:1080. undici passes the bracketed URL hostname to the socket connect, which failed with ENOTFOUND; a wrapped connector now strips the brackets. - Validate the percent-encoding of proxy user names and passwords while classifying proxy settings. undici decodes them during agent construction and threw URIError, so a malformed ALL_PROXY made every command, including --help, exit with only "URI malformed". Such an ALL_PROXY is now ignored with a redacted warning; an explicit HTTP_PROXY/HTTPS_PROXY fails with an error naming the variable. - Pass decoded SOCKS5 credentials as agent options and remove them from the proxy URL so undici does not decode them again.
hetaoBackend
approved these changes
Oct 9, 2026
hetaoBackend
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the diff and ran it locally (typecheck, lint, network-proxy*.test.ts: 61 passed). The new SOCKS5 tests fail against main, so they do cover the #393 regression. CI is green on all platforms, and nothing outside the network layer is touched.
The approach looks right: SOCKS5 goes through undici's Socks5ProxyAgent, an unusable ALL_PROXY is ignored with a single warning instead of aborting startup, credentials are masked in diagnostics, and NO_PROXY still bypasses the proxy.
Non-blocking:
- The PR checklist says shared docs were updated, but the diff has no doc changes. Please add a line about SOCKS5 support or adjust the checklist.
socks5://andsocks5h://both resolve hostnames at the proxy in undici, whereas curl resolves locally forsocks5://. Worth one sentence in the docs for anyone relying on local DNS.- The validation error lists
http://, https://, socks5://, or socks5h://, butsocks://is also accepted. List it or drop the alias. - The experimental-warning filter matches undici's exact message text, so a wording change upstream would just bring the warning back. Harmless. Also keep an eye on this when upgrading undici, since SOCKS5 is still experimental there.
- Bedrock with a SOCKS
ALL_PROXYstill fails at request time with an unsupported-protocol error rather than at startup; the PR says this is intentional.
Approving.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change
Fixes #393.
ALL_PROXY=socks5://...(a common setup with Clash and similar proxy apps) used to abort CLI startup withALL_PROXY must be an http:// or https:// URL., becauseresolveTuiProxyConfigurationfell back toALL_PROXYand only accepted http/https proxies.socks5://,socks5h://andsocks://proxies (viaALL_PROXYor explicitHTTP(S)_PROXY) now go through undici's built-inSocks5ProxyAgent, including username/password auth. Host names are resolved by the proxy (domain-name CONNECT), sosocks5h://is accepted as an alias.NO_PROXYhosts, including loopback, still connect directly.socks5://[::1]:1080work: the SOCKS5 agent uses a connector built withbuildConnectorthat strips the brackets before dialing (undici otherwise resolves[::1]and fails withENOTFOUND).50%off) no longer crash startup withURI malformed: anALL_PROXYfallback is ignored with a warning, and an explicitHTTP(S)_PROXYfails with an error naming the variable. Decoded credentials are passed toSocks5ProxyAgentasusername/passwordoptions rather than through the URL.ALL_PROXYfallback (for examplesocks4://or a malformed URL) is ignored with a single stderr warning that tells the user how to fix it, instead of failing startup. Explicit invalidHTTP_PROXY/HTTPS_PROXYvalues are still rejected.ExperimentalWarningis suppressed, and only while the agent is constructed; other warnings are unaffected.The global dispatcher configured here is not overridden by
third_party/pi-mono'sconfigureHttpDispatcher: it is only called from pi-mono's own CLI entry points, which mcode does not load.bug,tui,cliValidation
pnpm verify(full profile) on74ed9bfwith a clean tracked tree, darwin/arm64, Node 24.16.0: PASS (also passed onc510234).test:windowsskipped (platform),test:release-packageskipped (requires an npm release archive). Targeted:network-proxy.test.ts+network-proxy-socks5.test.ts, 61 tests passed. The new SOCKS5 tests fail onmainwith the [Bug]: CLI hard-fails on startup when ALL_PROXY=socks5://… #393 error.[::1]) and asserts domain-name CONNECT (ATYP0x03), auth success/failure, empty passwords, malformed-credential fallback at startup, and thatNO_PROXYhosts bypass the proxy.c510234): on macOS, a real no-auth SOCKS5 proxy from OpenSSH (ssh -N -D 1080 localhost) and onlyALL_PROXY=socks5://127.0.0.1:1080set,node dist/cli.js exec "Reply with exactly: socks ok"returnedsocks okwith exit code 0 and empty stderr (no SOCKS5 ExperimentalWarning). During the run,lsofshowed the mcode process holding connections to127.0.0.1:1080, confirming the traffic went through the proxy.third_party/pi-mono/packages/ai/src/utils/node-http-proxy.ts) still rejects SOCKS proxies and is intentionally untouched. undici marks SOCKS5 support as experimental.https://[::1]/), undici still sends the bracketed host name in the SOCKS5 CONNECT request. mcode only targets domain names, so it is not affected; this needs an upstream undici fix.Publication and contribution checks
release/public-source.json; new tests are declared intest/vitest-suites.jsonwhere applicable.Maintainer handoff
Publication scope or license changes (if any): none.
Shared-source port: pending.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.